Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace callable with Endpoint factory #237

Merged
merged 10 commits into from
Nov 15, 2024

Conversation

kimpepper
Copy link
Contributor

Description

As part of #233 we want to modernise the API in terms of getting endpoints.

This PR replaces the callable for getting endpoints with a strongly typed EndpointFactory.

In addition, the key for the endpoints is the fully qualified class name. This allows us to use the ::class constant (e.g. \OpenSearch\Endpoints\Bulk::class) which provides an improved DX, autocompletion, phpstan type checks etc.

The use of generics in the method docblock allows IDEs and phpstan to correctly assign the return type

    /**
     * Gets an endpoint.
     *
     * @phpstan-template T of AbstractEndpoint
     * @phpstan-param class-string<T> $class
     * @phpstan-return T
     */
    public function getEndpoint(string $class): AbstractEndpoint;

Issues Resolved

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@dblock
Copy link
Member

dblock commented Nov 4, 2024

Same as the other PR, let's look at the changes that are just the non-generated code? I merged #235 so you can rebase, but still make sure you're not modifying any generated code, only the templates used to generate it. If the project doesn't build/test anymore without those, at least make two separate commits (one of the change, the other for re-generating specs) so we can see the actual code changes in the diff.

@kimpepper
Copy link
Contributor Author

@dblock done ✔️

I'm not sure how the automated generation cron job works. Does it work on PRs too? Do you need to merge first?

It would be great if you could generate a specific commit hash of the spec rather that only the latest.

@shyim
Copy link
Collaborator

shyim commented Nov 5, 2024

I think we need to support both callback and your new factory. Otherwise it's an break

@kimpepper
Copy link
Contributor Author

OK we should we trigger a deprecation error then, and specify we will drop support of callable in the next major ie. 3.0.0?

@shyim
Copy link
Collaborator

shyim commented Nov 5, 2024

yes sounds good

@kimpepper
Copy link
Contributor Author

Added BC support for the deprecated $endpoints property.

src/OpenSearch/LegacyEndpointFactory.php Show resolved Hide resolved
src/OpenSearch/ClientBuilder.php Show resolved Hide resolved
src/OpenSearch/ClientBuilder.php Outdated Show resolved Hide resolved
src/OpenSearch/EndpointInterface.php Show resolved Hide resolved
src/OpenSearch/EndpointInterface.php Show resolved Hide resolved
src/OpenSearch/Traits/DeprecatedPropertyTrait.php Outdated Show resolved Hide resolved
src/OpenSearch/Traits/DeprecatedPropertyTrait.php Outdated Show resolved Hide resolved
src/OpenSearch/ClientBuilder.php Outdated Show resolved Hide resolved
src/OpenSearch/Traits/DeprecatedPropertyTrait.php Outdated Show resolved Hide resolved
util/template/client-class Outdated Show resolved Hide resolved
Copy link
Contributor Author

@kimpepper kimpepper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review. I've addressed your feedback and provided repllies where I disagree.

src/OpenSearch/Traits/DeprecatedPropertyTrait.php Outdated Show resolved Hide resolved
src/OpenSearch/Traits/DeprecatedPropertyTrait.php Outdated Show resolved Hide resolved
src/OpenSearch/Traits/DeprecatedPropertyTrait.php Outdated Show resolved Hide resolved
util/template/client-class Outdated Show resolved Hide resolved
util/template/client-class Outdated Show resolved Hide resolved
util/template/client-class Outdated Show resolved Hide resolved
src/OpenSearch/Traits/DeprecatedPropertyTrait.php Outdated Show resolved Hide resolved
src/OpenSearch/ClientBuilder.php Outdated Show resolved Hide resolved
Copy link

@JoshuaBehrens JoshuaBehrens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for approaching this. It looks really helpful. I just had to add a default parameter to bulk and index this week. With my approach (which is super hacky) I had to build up a lot of technical debt. Now I can just decorate the factory. And can use the shared serializer without making up one my own as the one used in a built Client instance is not retrievable.

Super hacky code
<?php

declare(strict_types=1);

namespace App\Test\Support\ElasticSearch;

use OpenSearch\Client;
use OpenSearch\ClientBuilder;
use OpenSearch\Endpoints\Bulk;
use OpenSearch\Endpoints\Index;
use OpenSearch\Serializers\SmartSerializer;
use Psr\Log\LoggerInterface;
use Shopware\Elasticsearch\Framework\ClientFactory;

/**
 * This factory ensures, that bulk and index steps are waiting to be completed on elasticsearch side.
 * This way the next request can expect to find the indexed documents in a search.
 *
 * @deprecated This is a bare copy with just a slight adjustment on the builder. Re-evaluate in case you update.
 */
final class WaitForBulkClientFactory extends ClientFactory
{
    /**
     * @param array{verify_server_cert: bool, cert_path?: string, cert_key_path?: string} $sslConfig
     */
    #[\Override]
    public static function createClient(string $hosts, LoggerInterface $logger, bool $debug, array $sslConfig): Client
    {
        $hosts = \array_filter(\explode(',', $hosts));

        $clientBuilder = ClientBuilder::create();
        $clientBuilder->setHosts($hosts);

        if ($debug) {
            $clientBuilder->setTracer($logger);
        }

        $clientBuilder->setLogger($logger);

        if ($sslConfig['verify_server_cert'] === false) {
            $clientBuilder->setSSLVerification(false);
        }

        if (isset($sslConfig['cert_path'])) {
            $clientBuilder->setSSLCert($sslConfig['cert_path'], $sslConfig['cert_password'] ?? null);
        }

        if (isset($sslConfig['cert_key_path'])) {
            $clientBuilder->setSSLKey($sslConfig['cert_key_path'], $sslConfig['cert_key_password'] ?? null);
        }

        // BEGIN CHANGE
        $clientBuilder->setEndpoint(function ($class) {
            $serializer = new SmartSerializer();

            if ($class === 'Bulk') {
                return new class($serializer) extends Bulk {
                    #[\Override]
                    public function setParams(array $params): void
                    {
                        // this is where the magic happens
                        $params['refresh'] ??= 'wait_for';

                        parent::setParams($params);
                    }
                };
            }

            if ($class === 'Index') {
                return new class() extends Index {
                    #[\Override]
                    public function setParams(array $params): void
                    {
                        // this is where the magic happens
                        $params['refresh'] ??= 'wait_for';

                        parent::setParams($params);
                    }
                };
            }

            $fullPath = '\\OpenSearch\\Endpoints\\' . $class;

            $reflection = new \ReflectionClass($fullPath);
            $constructor = $reflection->getConstructor();

            if ($constructor && $constructor->getParameters()) {
                return new $fullPath($serializer);
            } else {
                return new $fullPath();
            }
        });
        // END CHANGE

        return $clientBuilder->build();
    }
}

@shyim would you mind following up to do a change in shopware/elasticsearch to inject the endpoint factory via DI? Or shall I go there :)

util/template/client-class Outdated Show resolved Hide resolved
util/template/client-class Show resolved Hide resolved
util/template/client-class Outdated Show resolved Hide resolved
util/template/client-class Outdated Show resolved Hide resolved
util/template/client-class Outdated Show resolved Hide resolved
src/OpenSearch/Namespaces/AbstractNamespace.php Outdated Show resolved Hide resolved
@dblock
Copy link
Member

dblock commented Nov 13, 2024

I came here to say that I am so grateful to @kimpepper for picking the PSR work up and for @stof foe the CRs and @shyim for your support.

@dblock
Copy link
Member

dblock commented Nov 13, 2024

@stof looking at you to comment when you’re satisfied with the PR, @kimpepper iterate to green

CHANGELOG.md Outdated Show resolved Hide resolved
@stof
Copy link
Contributor

stof commented Nov 14, 2024

To make unit tests pass, you will need to regenerate the client and namespaces in this PR instead of changing only the templates. And you also need to remove the CI jobs running on PHP 7 due to the new PHP requirement.

@kimpepper
Copy link
Contributor Author

kimpepper commented Nov 14, 2024

@stof

To make unit tests pass, you will need to regenerate the client and namespaces in this PR instead of changing only the templates. And you also need to remove the CI jobs running on PHP 7 due to the new PHP requirement.

I think we should merge #238 first to avoid a load of unrelated changes.

PHP 7 was recently removed in #239

@dblock
Copy link
Member

dblock commented Nov 14, 2024

I think we should merge #238 first to avoid a load of unrelated changes.

Just did. That was a clean update from spec. Rebase.

kimpepper and others added 8 commits November 15, 2024 10:44
Signed-off-by: Kim Pepper <[email protected]>

Signed-off-by: Kim Pepper <[email protected]>

Signed-off-by: Kim Pepper <[email protected]>
Signed-off-by: Kim Pepper <[email protected]>
Signed-off-by: Kim Pepper <[email protected]>
Signed-off-by: Kim Pepper <[email protected]>
Signed-off-by: Kim Pepper <[email protected]>
Signed-off-by: Kim Pepper <[email protected]>
Signed-off-by: Kim Pepper <[email protected]>
Signed-off-by: Kim Pepper <[email protected]>
@kimpepper
Copy link
Contributor Author

OK pushed latest generated code and all tests are green ✔️

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, thanks @kimpepper! I'm going to merge this, @shyim do take a look in case something was amiss. @stof much thanks for the iterations/CRs.

@dblock dblock merged commit f110a17 into opensearch-project:main Nov 15, 2024
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants